Enable S3TC_BPTC but not ETC2_ASTC by default #77105

aaronfranke · 2023-05-15T17:00:15Z

Fixes #74609 (tested and working), regression from #72031 (probably)

~~The formats are now both enabled by default, which allows exporting for platforms that use either one. Note that which formats get included in the export depend on the export settings.~~ The S3TC_BPTC is now enabled by default, and ETC2_ASTC is disabled by default.

More importantly, this fixes an issue where x86 platforms (ex: Windows x86_64) and Arm platforms (ex: macOS arm64) would fight over the texture format. Setting either one enabled in project settings would be useless because the other platform would have a different default setting so it would be wiped from project.godot. Now the project settings do not depend on the OS so it will work across platforms and you won't get diffs and reimports.

clayjohn · 2023-05-15T18:02:33Z

The issue will still come back if one of these are disabled. I'd prefer not to import both formats by default as importing ETC/ASTC is way slower so you end up more than doubling the import time of textures.

Would it make sense to provide a .macos variant for these properties? We could add .mobile and ..macos which default to etc_astc and then everything else can default to s3tc_bptc.

aaronfranke · 2023-05-15T18:28:01Z

@clayjohn Changing the default setting based on the platform does not work and is precisely why this bug exists.

The imported texture formats are explicitly saved in the .import config files.

Every time we open our project on macOS, we get our .png.import files changed to this:

"imported_formats": ["s3tc_bptc", "etc2_astc"],

Every time we open our project on Windows or Linux, we get our .png.import files changed to this:

"imported_formats": ["s3tc_bptc"],

If we wanted the format to change depending on the OS, it must not be saved in the .import config files.

fire · 2023-05-15T19:02:10Z

We need to rethink this. As far as I know this doubles the exported image size and time complexity.

aaronfranke · 2023-05-15T19:26:56Z

@fire It doesn't affect exported images, that's controlled by the export presets. This only affects the imported images.

akien-mga · 2023-06-09T13:29:28Z

As we discussed on RC with @reduz, enabling both is too wasteful, but the default for macOS should be changed to S3TC_BPTC like on other desktop platforms, as it's supported by ARM Macs, and needed for Intel Macs.

aaronfranke · 2023-06-09T15:35:02Z

@akien-mga Changed as requested.

akien-mga · 2023-06-09T15:57:05Z

servers/rendering_server.cpp

@@ -2870,8 +2870,8 @@ TypedArray<StringName> RenderingServer::_global_shader_parameter_get_list() cons
 }

 void RenderingServer::init() {
-	GLOBAL_DEF_RST_NOVAL_BASIC("rendering/textures/vram_compression/import_s3tc_bptc", OS::get_singleton()->get_preferred_texture_format() == OS::PREFERRED_TEXTURE_FORMAT_S3TC_BPTC);
-	GLOBAL_DEF_RST_NOVAL_BASIC("rendering/textures/vram_compression/import_etc2_astc", OS::get_singleton()->get_preferred_texture_format() == OS::PREFERRED_TEXTURE_FORMAT_ETC2_ASTC);


Shouldn't we change the preferred OS texture format on macOS instead? This will likely make it still look up ETC2/ASTC and thus not work on Intel Mac.

Also this might break the Android editor.

Yes, we should also do that. But not instead. To be clear, the project settings themselves must not change between platforms. The behavior can change, sure, but not the booleans themselves, or else you get diffs in project.godot when you switch between platforms.

Users of the Android editor will need this setting enabled to avoid .import file diffs compared to desktop platforms, however even with this setting disabled Android will still import what it needs, you'll just get diffs in the .import files. However the difference is that with this setting enabled you can enforce it being imported on all platforms, while in the current master Godot just automatically deletes the setting from project.godot, making it useless.

akien-mga · 2023-06-09T23:04:25Z

Thanks!

reduz · 2023-06-11T16:42:58Z

Just a note that this PR is incorrect and it will break the Android and web editors, as well as Linux on ARM.
The correct fix would have been simply these lines:

OS::PreferredTextureFormat OS_MacOS::get_preferred_texture_format() const {
	// macOS supports both formats on ARM. Prefer S3TC/BPTC
	// for better compatibility with x86 platforms.
	return PREFERRED_TEXTURE_FORMAT_S3TC_BPTC;
}

and nothing else. Everything else this PR adds is unnecessary. I suggest reverting.

aaronfranke · 2023-06-11T17:59:06Z

@reduz I will explain again. The default values of the project settings booleans themselves MUST NOT change between platforms. If they do, the setting is erased by Godot whenever you switch platforms. The setting in project.godot is an override to force it to be enabled, it is not and cannot be the definitive source.

aaronfranke added bug platform:macos topic:import high priority regression labels May 15, 2023

aaronfranke added this to the 4.1 milestone May 15, 2023

aaronfranke requested review from a team as code owners May 15, 2023 17:00

fire requested a review from a team May 15, 2023 18:28

aaronfranke requested a review from reduz May 28, 2023 06:54

aaronfranke force-pushed the s3tc_bptc_and_etc2_astc branch from 8059151 to 3a604bb Compare June 4, 2023 04:31

aaronfranke force-pushed the s3tc_bptc_and_etc2_astc branch 2 times, most recently from 86a15b2 to c1b7601 Compare June 9, 2023 15:33

aaronfranke changed the title ~~Enable both S3TC_BPTC and ETC2_ASTC import by default~~ Enable S3TC_BPTC but not ETC2_ASTC by default Jun 9, 2023

akien-mga reviewed Jun 9, 2023

View reviewed changes

Enable S3TC_BPTC but not ETC2_ASTC by default

944fbce

aaronfranke force-pushed the s3tc_bptc_and_etc2_astc branch from c1b7601 to 944fbce Compare June 9, 2023 16:39

aaronfranke requested a review from a team as a code owner June 9, 2023 16:39

akien-mga approved these changes Jun 9, 2023

View reviewed changes

akien-mga merged commit 9723077 into godotengine:master Jun 9, 2023

aaronfranke deleted the s3tc_bptc_and_etc2_astc branch June 9, 2023 23:39

aaronfranke mentioned this pull request Jun 12, 2023

Set both texture format overrides to false by default #78147

Merged

aaronfranke mentioned this pull request Nov 18, 2023

Constant project.godot and re-importing when using MacOS and Windows #74609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable S3TC_BPTC but not ETC2_ASTC by default #77105

Enable S3TC_BPTC but not ETC2_ASTC by default #77105

aaronfranke commented May 15, 2023 •

edited

Loading

clayjohn commented May 15, 2023

aaronfranke commented May 15, 2023

fire commented May 15, 2023 •

edited

Loading

aaronfranke commented May 15, 2023

akien-mga commented Jun 9, 2023

aaronfranke commented Jun 9, 2023 •

edited

Loading

akien-mga Jun 9, 2023

aaronfranke Jun 9, 2023 •

edited

Loading

akien-mga commented Jun 9, 2023

reduz commented Jun 11, 2023 •

edited

Loading

aaronfranke commented Jun 11, 2023 •

edited

Loading

Enable S3TC_BPTC but not ETC2_ASTC by default #77105

Enable S3TC_BPTC but not ETC2_ASTC by default #77105

Conversation

aaronfranke commented May 15, 2023 • edited Loading

clayjohn commented May 15, 2023

aaronfranke commented May 15, 2023

fire commented May 15, 2023 • edited Loading

aaronfranke commented May 15, 2023

akien-mga commented Jun 9, 2023

aaronfranke commented Jun 9, 2023 • edited Loading

akien-mga Jun 9, 2023

Choose a reason for hiding this comment

aaronfranke Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

akien-mga commented Jun 9, 2023

reduz commented Jun 11, 2023 • edited Loading

aaronfranke commented Jun 11, 2023 • edited Loading

aaronfranke commented May 15, 2023 •

edited

Loading

fire commented May 15, 2023 •

edited

Loading

aaronfranke commented Jun 9, 2023 •

edited

Loading

aaronfranke Jun 9, 2023 •

edited

Loading

reduz commented Jun 11, 2023 •

edited

Loading

aaronfranke commented Jun 11, 2023 •

edited

Loading